feat: redis adapter for entity caching cache - #3139
Conversation
WalkthroughAdds a Redis-backed entity cache with namespaced keys, pipelined batch reads and writes, TTL handling, partial-write errors, context support, and comprehensive Miniredis tests. It also clarifies an in-memory cache test for invalid TTL validation. ChangesRedis cache
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
SkArchon
changed the title
Router image scan passed✅ No security vulnerabilities found in image: |
3845440 to
3562724
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3139 +/- ##
==========================================
+ Coverage 62.40% 62.47% +0.06%
==========================================
Files 263 264 +1
Lines 31048 31098 +50
==========================================
+ Hits 19376 19428 +52
+ Misses 10162 10159 -3
- Partials 1510 1511 +1
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
router/pkg/entitycaching/cache/redis_test.go (1)
203-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test does not test what its name says.
The subtest is named "cache tags are accepted and ignored". The item carries no tag field, and no assertion refers to tags. The body is identical to "namespaces keys with the prefix" at Lines 108-119.
Either remove the subtest, or add a real tag field to the item once the cache item type exposes one.
♻️ Proposed removal
- t.Run("cache tags are accepted and ignored", func(t *testing.T) { - t.Parallel() - - c, mr := newTestRedisCache(t) - - err := c.SetMany(ctx, []enginecache.Item{ - { - Key: "a", - Value: []byte("value"), - TTL: time.Minute, - }, - }) - require.NoError(t, err) - - require.Equal(t, []string{testPrefix + "a"}, mr.Keys()) - }) -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/entitycaching/cache/redis_test.go` around lines 203 - 218, Remove the redundant “cache tags are accepted and ignored” subtest from the Redis cache tests, since its item has no tags and it duplicates the existing namespace-prefix coverage.router/pkg/entitycaching/cache/redis.go (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueError context is inconsistent between the two failure paths.
If
Execreports a command error other thanredis.Nil,GetManyreturns it unwrapped. The per-command loop below wraps the same class of error with the failing key. The result is that identical Redis failures produce different messages depending on whether aredis.Nilmiss appears earlier in the batch. The test atredis_test.goLines 374-388 depends on that ordering detail to get the key into the message.Consider dropping the
Execerror early return for command-level failures and letting the per-command loop classify every error, so the key is always present. Keep a check for transport-level failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/entitycaching/cache/redis.go` around lines 47 - 50, Update GetMany so the pipe.Exec error handling does not return command-level failures before the per-command loop can add the failing key; retain only the transport-level failure check at the Exec stage. Let the existing per-command classification and wrapping logic handle each command error consistently, including batches containing redis.Nil misses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/pkg/entitycaching/cache/redis.go`:
- Around line 52-63: Update GetMany’s successful StringCmd.Bytes() result
handling to assign bytes.Clone(value) to enginecache.Result.Value, ensuring
returned values do not alias Redis command storage while preserving the existing
error and missing-key behavior.
---
Nitpick comments:
In `@router/pkg/entitycaching/cache/redis_test.go`:
- Around line 203-218: Remove the redundant “cache tags are accepted and
ignored” subtest from the Redis cache tests, since its item has no tags and it
duplicates the existing namespace-prefix coverage.
In `@router/pkg/entitycaching/cache/redis.go`:
- Around line 47-50: Update GetMany so the pipe.Exec error handling does not
return command-level failures before the per-command loop can add the failing
key; retain only the transport-level failure check at the Exec stage. Let the
existing per-command classification and wrapping logic handle each command error
consistently, including batches containing redis.Nil misses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4fbd543-f34b-46c0-92d8-b9fbf1d6e9e1
📒 Files selected for processing (2)
router/pkg/entitycaching/cache/redis.gorouter/pkg/entitycaching/cache/redis_test.go
| // RedisCache stores entries in Redis. Redis owns expiry, so unlike | ||
| // InMemoryCache there is nothing to sweep here. |
There was a problem hiding this comment.
I wouldn't mention the in memory store here. Has nothing to do with this implementation + I always like to mention the interface this intends to implement.
| // RedisCache stores entries in Redis. Redis owns expiry, so unlike | |
| // InMemoryCache there is nothing to sweep here. | |
| // RedisCache stores entries in Redis. It implements enginecache.Cache |
| // InMemoryCache there is nothing to sweep here. | ||
| type RedisCache struct { | ||
| client redis.UniversalClient | ||
| prefix string |
There was a problem hiding this comment.
Can you add godoc to fields as well? I.e. whats prefix good for?
| } | ||
|
|
||
| // A miss surfaces as redis.Nil, which is not a failure of the batch. | ||
| if _, err := pipe.Exec(ctx); err != nil && !errors.Is(err, redis.Nil) { |
There was a problem hiding this comment.
Nit: too much happening in one line. Can you split it up?
| if _, err := pipe.Exec(ctx); err != nil && !errors.Is(err, redis.Nil) { | |
| _, err := pipe.Exec(ctx) | |
| if err != nil && !errors.Is(err, redis.Nil) { |
| // There is no partial read to salvage, the whole batch fails. | ||
| return nil, fmt.Errorf("get %q: %w", keys[i], err) | ||
| } | ||
| results[i] = enginecache.Result{Value: value, Found: true} |
There was a problem hiding this comment.
cmd.Bytes() does not copy the value but instead returns the value as an unsafe.Pointer. As you return this value its possible someone outside this package will modify the value under the hood. Better copy it.
| results[i] = enginecache.Result{Value: value, Found: true} | |
| results[i] = enginecache.Result{Value: bytes.Clone(value), Found: true} |
| func TestRedisCache(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx := context.Background() |
| require.Empty(t, mr.Keys()) | ||
| }) | ||
|
|
||
| t.Run("one bad item rejects the whole batch", func(t *testing.T) { |
There was a problem hiding this comment.
This test contradicts the contract defined in the godoc of SetMany where it states "An error means an unspecified subset of the items may already have been stored"
There was a problem hiding this comment.
This is for ttl pre-validation, will rename
| require.Empty(t, mr.Keys()) | ||
| }) | ||
|
|
||
| t.Run("cache tags are accepted and ignored", func(t *testing.T) { |
There was a problem hiding this comment.
Where exactly are cache tags used in this test? I don't know much about these tags but it does not look obvious from looking at the test
There was a problem hiding this comment.
This test should be removed, I removed the cache tag field for now.
| err := c.SetMany(ctx, []enginecache.Item{ | ||
| {Key: "a", Value: []byte("value"), TTL: time.Minute}, | ||
| }) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Use assert.Contains to broadly verify the error is about failure to reach redis
3562724 to
1df9830
Compare
Moves the redis cache into its own package and brings it in line with the updated Cache interface. GetMany now returns map[string]Item, pairing a PTTL with every GET in the same pipeline so each hit carries the lifetime it has left. A PTTL of -2, -1 or 0 is treated as a miss, so a hit always has a usable TTL: the pair is not atomic, a key with no expiry was not written by SetMany, and a sub-millisecond remainder is nothing a caller can act on. SetMany keeps each command alongside the item that queued it, so a failed batch can report the keys redis actually answered in a SetManyError. That list understates what was written and never overstates it, since a command left carrying a transport error may have been applied with its reply lost.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
router/pkg/entitycaching/cache/in_memory/in_memory_test.go (1)
664-677: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the busy-wait loop.
Line 664 calls
GetManycontinuously for up toeventuallyFor. The surrounding parallel tests can lose CPU time while this loop runs. Sleep or wait on a ticker after each successful hit.Proposed fix
require.Positive(t, item.TTL) + time.Sleep(eventuallyTick) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/entitycaching/cache/in_memory/in_memory_test.go` around lines 664 - 677, Update the expiration polling loop around c.GetMany to avoid continuous busy-waiting: after each successful hit, wait briefly using a sleep or ticker before retrying, while retaining the existing deadline, TTL assertion, and vanished-entry checks.router/pkg/entitycaching/cache/redis/redis_test.go (2)
823-823: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the interface assertion into
redis.go.The assertion sits in a test file, so only a test build enforces it.
enginecache.Cacheis defined in an external module. If an upgrade adds a method,go build ./...still succeeds and the break surfaces later. Place the assertion inredis.go.♻️ Proposed move
In
router/pkg/entitycaching/cache/redis/redis_test.go:-var _ enginecache.Cache = (*RedisCache)(nil)In
router/pkg/entitycaching/cache/redis/redis.go, after theRedisCachedeclaration:var _ enginecache.Cache = (*RedisCache)(nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/entitycaching/cache/redis/redis_test.go` at line 823, Move the compile-time interface assertion `var _ enginecache.Cache = (*RedisCache)(nil)` from the test file into `redis.go`, placing it immediately after the `RedisCache` declaration so regular builds validate the implementation against `enginecache.Cache`.
349-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the partial-write count directly
SetManyError.Error()currently contains"1 keys were known to be written", but this text is owned upstream. Assert the unwrappedSetManyError.KnownStoredKeyscount instead, while retainingrequire.ErrorIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/entitycaching/cache/redis/redis_test.go` at line 349, Update the test assertion around SetManyError to unwrap the error and assert the KnownStoredKeys count directly, rather than matching the upstream error message text. Retain the existing require.ErrorIs assertion and verify the expected partial-write count is 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/pkg/entitycaching/cache/redis/redis.go`:
- Line 39: Update the GetMany doc comment to describe that it returns a map
keyed by the caller-provided keys, omitting missing or expired entries; do not
claim ordered or one-result-per-key behavior. Also correct the nearby field
comment to state that results use the original caller keys and that the Redis
prefix is not stripped.
---
Nitpick comments:
In `@router/pkg/entitycaching/cache/in_memory/in_memory_test.go`:
- Around line 664-677: Update the expiration polling loop around c.GetMany to
avoid continuous busy-waiting: after each successful hit, wait briefly using a
sleep or ticker before retrying, while retaining the existing deadline, TTL
assertion, and vanished-entry checks.
In `@router/pkg/entitycaching/cache/redis/redis_test.go`:
- Line 823: Move the compile-time interface assertion `var _ enginecache.Cache =
(*RedisCache)(nil)` from the test file into `redis.go`, placing it immediately
after the `RedisCache` declaration so regular builds validate the implementation
against `enginecache.Cache`.
- Line 349: Update the test assertion around SetManyError to unwrap the error
and assert the KnownStoredKeys count directly, rather than matching the upstream
error message text. Retain the existing require.ErrorIs assertion and verify the
expected partial-write count is 1.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dacfb96a-cbd3-444f-a131-4260fa33d8cb
📒 Files selected for processing (3)
router/pkg/entitycaching/cache/in_memory/in_memory_test.gorouter/pkg/entitycaching/cache/redis/redis.gorouter/pkg/entitycaching/cache/redis/redis_test.go
| return &RedisCache{client: client, prefix: prefix}, nil | ||
| } | ||
|
|
||
| // GetMany returns one result per key, in the same order as keys. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the GetMany doc comment.
GetMany returns a map[string]enginecache.Item. A map has no order, and misses and expired entries are omitted, so there is not one result per key. Describe the actual contract.
The field comment at lines 21-22 has the same problem. The prefix is never stripped. Results are keyed by the caller key, which was never prefixed.
📝 Proposed doc fix
-// GetMany returns one result per key, in the same order as keys.
+// GetMany returns an entry for every key that is present with a positive
+// remaining TTL, keyed by the caller's key. Misses and expired entries are
+// absent from the map and are not an error. // prefix is prepended to every key before it reaches redis, so entity cache
// entries stay in their own namespace and cannot collide with anything else
- // sharing the instance. It is applied on the way in and stripped back off on
- // the way out, so callers only ever see the keys they asked with. An empty
- // prefix is valid and means the keys are used as they are.
+ // sharing the instance. It is applied on the way in only; results are keyed
+ // by the caller's key, so callers never see the prefix. An empty prefix is
+ // valid and means the keys are used as they are.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/pkg/entitycaching/cache/redis/redis.go` at line 39, Update the GetMany
doc comment to describe that it returns a map keyed by the caller-provided keys,
omitting missing or expired entries; do not claim ordered or one-result-per-key
behavior. Also correct the nearby field comment to state that results use the
original caller keys and that the Redis prefix is not stripped.
2 participants
This PR contains a redis adapter to be used for entity caching, currently it does not have support for managing cache tags.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.